XDM: Relayer and Fraud proof updates #2562

vedhavyas · 2024-02-27T12:05:36Z

This PR brings following updates:

Update relayer to generate XDM proofs coming from both Consensus and Domain chain and gossips them
Enable Fraud proof verification for XDM
Enable tests on pallet-messenger

Note:
The first commit is a little packed due to very invasive changes to relayer. Initial was two commits but did not make any sense keeping the changes separate and instead merged to one.

With this Cross domain messaging can be enabled for testing but there are few more issues we need to handle

Balance reserve for Channel open
Permissioned channel opening between chains before we enable permissionless domains
Prune old MMR leaves through XDM message expiration

Code contributor checklist:

I have read, understood and followed contributing guide

…key fetch, and enable XDM test

…m fraud proof test

NingLin-P

Make sense in general

domains/client/relayer/src/worker.rs

NingLin-P · 2024-02-29T15:57:28Z

domains/client/relayer/src/lib.rs

+
+        Ok(Proof::Consensus {
+            consensus_chain_mmr_proof: ConsensusChainMmrLeafProof {
+                consensus_block_hash: finalized_block.1,


This field seems never used.

This is unused because the MMR proof is self contained but I wanted to keep the hash of finalized block for light client or other who are tracking MMR roots to be able to verify the proof stateless

domains/client/relayer/src/lib.rs

NingLin-P · 2024-02-29T18:59:13Z

domains/client/relayer/src/worker.rs

-                .into_iter()
-                .filter(|(number, _)| *number <= relay_block_until)
-                .collect();
+            Relayer::fetch_unprocessed_consensus_blocks_until(&client, chain_id, number, hash)?;


Not related to this PR but I'm thinking about we may not really need to track and process every block since the XDM is not extracted from the block body but instead from the state, and as long as the XDM is not responded it won't be removed from the state.

What we want is some retry in case the XDM is dropped silently in the network during relaying, and we can always retry with the latest finalized consensus block when the notification is come.

XDM will remain in the state when un responded, I'm not sure I clearly follow what you mean. As and when a consensus block is finalized, we relay the XDM from those blocks.

I realized we still need a fallback mechanism for when the message was dropped sliently. What we can do here is to run a simultaneous process that re-submits the XDM. I will handle this in seperate PR and there is an issue created for this already I believe

XDM will remain in the state when un responded, I'm not sure I clearly follow what you mean

I mean we don't need the aux storage for tracking whether a block is processed or not.

Since the XDM is available in the state, whenever a consensus block import/finalization notification comes, we can get the XDM from the state based on the imported/finalized block, filter already relayed XDM, and relay the XDM. If an XDM is dropped silently during propagation then when the next consensus block import/finalization notification comes we will retry to relay this XDM without needing another worker. And yeah let's handle this in a seperate PR.

NingLin-P · 2024-02-29T19:07:47Z

domains/client/relayer/src/worker.rs

+                // check if this domain block is already relayed
+                if Relayer::fetch_domains_blocks_relayed_at(
+                    &domain_client,
+                    domain_id,
+                    domain_block_number,
+                )
+                    .contains(&domain_block_hash)
+                {
+                    return Ok(None);
+                }


An edge case here, if the domain stops progressing (like not activity) then we only try to relay the XDM in the last confirmed domain block for once then mark the block processed and skip it afterward without retrying.

True. We need a fallback mechanism that is not built in yet as I have mentioned in the other comment. Will handle that fallback mechanism when we do a cleanup of the aux storage

NingLin-P · 2024-02-29T19:23:24Z

domains/client/relayer/src/worker.rs

    // then fetch new messages in the block
    // construct proof of each message to be relayed
    // submit XDM as unsigned extrinsic.
-    while let Some(block) = chain_block_import.next().await {
+    while let Some(block) = chain_block_finalization.next().await {


IIUC there only be one consensus block finalization notification per segment, so the finalized consensus block will not come like #1,#2,.. but instead #1000,#2000,.. cc @nazar-pc, perhaps use the block import notification with something like block_to_process = imported_block - K

That is fine. Relayer does not expect the block come iincremental. We just need the latest finalized block and we process all the blocks that in between last processed to latest finalized.

This may cause problem, if there is one new segment per 1 hour then all relayers from any domain will idle for 1 hour then start relaying XDM at the same time, which brings additional delay and network congestion.

Yes that is correct, XDM is already quiet delayed due to block confirmation and we expliclty want to wait until consensus block is finalized.

notification with something like block_to_process = imported_block - K

This approach defeats the purpose of waiting for consensus block finalization but rather relying on Kdepth. I'm not sure why we moved away from it but I would like to still relay on finalization instead of block_import

I agree using block finalization is better in term of readability, but the consensus block finalization is coupled with archiving and differs from what is provided by substrate by default. Besides the above concern about additional delay and network congestion, also consider if an XDM is dropped silently it needs to wait for another new segment before the next retry.

also consider if an XDM is dropped silently it needs to wait for another new segment before the next retry.

I dont see any concern here. There will always be a retry of the XDM in the next turn and XDM will end being included in the chain. This delay is what we opted for as a base XDM using confirmation and finalization.
We expect external services to be built on top to speed up these transfers at higher incentive and is not a concern for the Subspace.

BUt feel free to propose any potential alternatives on making this approach faster and I would like to understand what can be done here better

There will always be a retry of the XDM in the next turn and XDM will end being included in the chain.

The difference is one retry per block versus one retry per segment.

This delay is what we opted for as a base XDM using confirmation and finalization.
BUt feel free to propose any potential alternatives on making this approach faster and I would like to understand what can be done here better

IMO, the hard required delay is: domain block confirmation (challenge period) + consensus block confirmation (K block depth) while the consensus block finalization is: K blocks depth + blocks of one segment. Simply using the consensus block import notification + K block depth can make it faster, non-blocker.

IMO, the hard required delay is: domain block confirmation (challenge period) + consensus block confirmation (K block depth)

I disagree. The hard requirement for XDM v2 is safety over fastness. This means, finalization rather than using K-depth.

From my understanding, K blocks depth is when a consensus block is confirmed and inrevertable which is what we need for XDM (and also used in many other places), while finalization is coupled with the archiving process to ensure the block is not pruned before archiving, finalization take much longer than K blocks but doesn't mean confirmed and non-finalized block is revertable. cc @nazar-pc in case my understanding is wrong.

Non-blocker for using finalization block as the additional delay is not a concern, plz resolve the conflict though.

…s client

nazar-pc · 2024-03-06T19:47:18Z

domains/client/relayer/src/lib.rs

@@ -1,21 +1,22 @@
 #![warn(rust_2018_idioms)]
+#![deny(unused_crate_dependencies)]


I'll have to remove this line. It results in following confusing error:

error: external crate `alloc` unused in `domain_client_message_relayer`: remove the dependency or add `use alloc as _;` | note: the lint level is defined here --> domains/client/relayer/src/lib.rs:2:9 | 2 | #![deny(unused_crate_dependencies)] | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: external crate `compiler_builtins` unused in `domain_client_message_relayer`: remove the dependency or add `use compiler_builtins as _;` error: external crate `panic_unwind` unused in `domain_client_message_relayer`: remove the dependency or add `use panic_unwind as _;` error: external crate `proc_macro` unused in `domain_client_message_relayer`: remove the dependency or add `use proc_macro as _;` error: external crate `test` unused in `domain_client_message_relayer`: remove the dependency or add `use test as _;` error: could not compile `domain-client-message-relayer` (lib test) due to 3 previous errors warning: build failed, waiting for other jobs to finish... error: could not compile `domain-client-message-relayer` (lib) due to 5 previous errors

To reproduce -Z build-std is needed, for example:

cd domains/client/relayer cargo clippy --all-targets -Z build-std --target x86_64-unknown-linux-gnu

I'm not sure where it comes from because there is no such things in the crate and I don't think any of the very few macros generate it either (I checked macro expansion). Might be compiler issue: rust-lang/rust#122105

vanhauser-thc · 2024-04-05T13:58:05Z

relevant: #2660
LGTM otherwise however due to the overall high complexity we will do a deep dive into XDM once the feature is stable.

update relayer to generate XDM proof with MMR, fix messenger storage …

2c10b20

…key fetch, and enable XDM test

vedhavyas requested review from NingLin-P, nazar-pc and rg3l3dr as code owners February 27, 2024 12:05

vedhavyas added 2 commits February 27, 2024 18:42

add fraud proof verification for XDM on domains and enable invalid xd…

7223b09

…m fraud proof test

update and enable pallet-messenger tests

24dcb50

vedhavyas force-pushed the xdm_mmr_2 branch from 51352b9 to 24dcb50 Compare February 27, 2024 13:19

NingLin-P reviewed Feb 29, 2024

View reviewed changes

make domain proof comment more specific and rename client to consensu…

e47dc52

…s client

vedhavyas requested a review from NingLin-P March 1, 2024 05:26

vedhavyas added 2 commits March 1, 2024 11:05

Merge branch 'main' into xdm_mmr_2

37d09b6

Merge branch 'main' into xdm_mmr_2

bfb2d38

vedhavyas force-pushed the xdm_mmr_2 branch from 4b3907d to bfb2d38 Compare March 5, 2024 05:24

vedhavyas enabled auto-merge March 5, 2024 05:26

NingLin-P approved these changes Mar 5, 2024

View reviewed changes

vedhavyas added this pull request to the merge queue Mar 5, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 5, 2024

vedhavyas added this pull request to the merge queue Mar 5, 2024

Merged via the queue into main with commit 0d22229 Mar 5, 2024
11 checks passed

vedhavyas deleted the xdm_mmr_2 branch March 5, 2024 17:28

nazar-pc reviewed Mar 6, 2024

View reviewed changes

vedhavyas added the need to audit This change needs to be audited label Apr 5, 2024

vanhauser-thc added audited This change was audited and removed need to audit This change needs to be audited labels Apr 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XDM: Relayer and Fraud proof updates #2562

XDM: Relayer and Fraud proof updates #2562

vedhavyas commented Feb 27, 2024 •

edited

Loading

NingLin-P left a comment

NingLin-P Feb 29, 2024

vedhavyas Mar 1, 2024

NingLin-P Feb 29, 2024

vedhavyas Mar 1, 2024

NingLin-P Mar 1, 2024

NingLin-P Feb 29, 2024

vedhavyas Mar 1, 2024

NingLin-P Feb 29, 2024

vedhavyas Mar 1, 2024

NingLin-P Mar 1, 2024

vedhavyas Mar 4, 2024

NingLin-P Mar 4, 2024

vedhavyas Mar 4, 2024

NingLin-P Mar 4, 2024

vedhavyas Mar 4, 2024

NingLin-P Mar 4, 2024

nazar-pc Mar 6, 2024

vanhauser-thc commented Apr 5, 2024

		@@ -1,21 +1,22 @@
		#![warn(rust_2018_idioms)]
		#![deny(unused_crate_dependencies)]

XDM: Relayer and Fraud proof updates #2562

XDM: Relayer and Fraud proof updates #2562

Conversation

vedhavyas commented Feb 27, 2024 • edited Loading

Code contributor checklist:

NingLin-P left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanhauser-thc commented Apr 5, 2024

vedhavyas commented Feb 27, 2024 •

edited

Loading